Make RapidsMPF the default runtime for cudf_polars streaming executor - #22281
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 4a37ee2 |
|
/ok to test 3e4dfe0 |
|
/ok to test 49849d0 |
|
/ok to test bc92082 |
bdice
left a comment
There was a problem hiding this comment.
Devcontainers look better now. I approve as long as CI passes.
| ) | ||
| for i, key in enumerate(partition_info[ir].keys(ir)) | ||
| } | ||
| return evaluate_rapidsmpf(ir, config_options) |
There was a problem hiding this comment.
In a followup we should merge evaluate_rapidsmpf and evaluate_streaming...
| # Target about 12.5% of the device memory when | ||
| # default_target_partition_size is used to set the | ||
| # target partition size (i.e. 5x the 2.5% default). | ||
| return min(5, int(max(1, (device_size * 0.125) // 1e9))) |
There was a problem hiding this comment.
TODO: I think we want to move towards making these limits all byte-based, rather than partition count.
There was a problem hiding this comment.
Yes, I agree. I proposed this verbally on Monday, and had a feeling you'd agree. It seems unnecessarily complicated tie target_partition_size and broadcast_join_limit together in the way we currently do. After this PR goes in, we may want to consider replacing broadcast_join_limit with a byte-based broadcast_limit before 26.02.
| client_device_threshold | ||
| Threshold for spilling data from device memory in rapidsmpf. | ||
| Threshold for spilling data from device memory. |
There was a problem hiding this comment.
nit: Why is client in this name?
There was a problem hiding this comment.
I think it's only used for the "single" cluster case and can be removed when we remove that code path (in favor of "spmd").
bdice
left a comment
There was a problem hiding this comment.
Approving to unblock, some final questions.
| sccache --zero-stats; | ||
| build-all -j0 -DBUILD_BENCHMARKS=ON --verbose 2>&1 | tee telemetry-artifacts/build.log; | ||
| clone-rapidsmpf -j$(nproc) -v -q --branch "$(cat ~/cudf/RAPIDS_BRANCH)" --clone-upstream --depth 1 --single-branch --shallow-submodules; | ||
| if [ "$PYTHON_PACKAGE_MANAGER" = "pip" ]; then rapids-make-pip-env --force; fi; |
There was a problem hiding this comment.
I feel like you would need the same thing in conda, since rapidsmpf is being cloned late. Let’s check on the dependency list and make sure it doesn’t have rapidsmpf or cudf before merging.
There was a problem hiding this comment.
Yeah, fair point. In fe5502b, the final, recreated conda environment creation doesn't include rapidsmpf
| build-all -j0 -DBUILD_BENCHMARKS=ON --verbose 2>&1 | tee telemetry-artifacts/build.log; | ||
| clone-rapidsmpf -j$(nproc) -v -q --branch "$(cat ~/cudf/RAPIDS_BRANCH)" --clone-upstream --depth 1 --single-branch --shallow-submodules; | ||
| if [ "$PYTHON_PACKAGE_MANAGER" = "pip" ]; then rapids-make-pip-env --force; fi; | ||
| build-all -j0 -DBUILD_BENCHMARKS=OFF -DBUILD_NUMA_SUPPORT=OFF -DBUILD_TESTS=OFF -DCUDF_BUILD_TESTUTIL=OFF --verbose 2>&1 | tee telemetry-artifacts/build.log; |
There was a problem hiding this comment.
We might need to call rapids-generate-scripts. I am not sure if build-all will do everything unless the scripts were regenerated.
There was a problem hiding this comment.
Sure I added it, but in fe5502b I can't tell if it materially made a difference
|
/merge |
…NVIDIA#22281) closes NVIDIA#21466 closes NVIDIA#21767 Waiting for NVIDIA#22212 * Makes rapidsmpf a required dependency of cudf_polars * Removes the following `StreamingExecutor` options as they were "experimental" with associated code paths * `StreamingExecutor.runtime` * `StreamingExecutor.shuffle_method` * `StreamingExecutor.unique_fraction` * `StreamingExecutor.groupby_n_ary` * `StreamingExecutor.rapidsmpf_spill` * Removes the task runtime and associated tests * Some tests we modified to only test 1 specific test configuration because of NVIDIA#22346 to pass these tests for now. Planning on revisiting this once rapidsmpf becomes the default Ops-Bot-Merge-Barrier: true Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Bradley Dice (https://github.com/bdice) - Matthew Murray (https://github.com/Matt711) - Lawrence Mitchell (https://github.com/wence-) URL: NVIDIA#22281
…lars (#22417) * Address the follow up review #22281 (comment) > In a followup we should merge evaluate_rapidsmpf and evaluate_streaming... * Removes a temporary iterable in `select` that I had from a leftover branch * Removes the `devcontainers` key in `dependencies.yml` that was leftover from troubleshooting the devcontainer CI job Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - James Lamb (https://github.com/jameslamb) - Peter Andreas Entschev (https://github.com/pentschev) URL: #22417
…NVIDIA#22281) closes NVIDIA#21466 closes NVIDIA#21767 Waiting for NVIDIA#22212 * Makes rapidsmpf a required dependency of cudf_polars * Removes the following `StreamingExecutor` options as they were "experimental" with associated code paths * `StreamingExecutor.runtime` * `StreamingExecutor.shuffle_method` * `StreamingExecutor.unique_fraction` * `StreamingExecutor.groupby_n_ary` * `StreamingExecutor.rapidsmpf_spill` * Removes the task runtime and associated tests * Some tests we modified to only test 1 specific test configuration because of NVIDIA#22346 to pass these tests for now. Planning on revisiting this once rapidsmpf becomes the default Ops-Bot-Merge-Barrier: true Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Bradley Dice (https://github.com/bdice) - Matthew Murray (https://github.com/Matt711) - Lawrence Mitchell (https://github.com/wence-) URL: NVIDIA#22281
…lars (NVIDIA#22417) * Address the follow up review NVIDIA#22281 (comment) > In a followup we should merge evaluate_rapidsmpf and evaluate_streaming... * Removes a temporary iterable in `select` that I had from a leftover branch * Removes the `devcontainers` key in `dependencies.yml` that was leftover from troubleshooting the devcontainer CI job Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - James Lamb (https://github.com/jameslamb) - Peter Andreas Entschev (https://github.com/pentschev) URL: NVIDIA#22417
…y; add [dask] pip extra (#22480) * Follow up to making rapidsmpf a required dependency of cudf-polars, #22281, we no longer need a `cudf-polars[rapidsmpf]` pip extra * Removes `numpy` as a testing dependency as it was only used for `np.random` and `np.full` * Removes `"nvidia-ml-py>=12"` from the `experimental` extra as it's already a required dependency of cudf_polars * Adds `cudf-polars[dask]` as an alias for `cudf-polars[experimental]` Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Tom Augspurger (https://github.com/TomAugspurger) - Bradley Dice (https://github.com/bdice) - Matthew Murray (https://github.com/Matt711) - Gil Forsyth (https://github.com/gforsyth) URL: #22480
Description
closes #21466
closes #21767
Waiting for #22212
StreamingExecutoroptions as they were "experimental" with associated code pathsStreamingExecutor.runtimeStreamingExecutor.shuffle_methodStreamingExecutor.unique_fractionStreamingExecutor.groupby_n_aryStreamingExecutor.rapidsmpf_spillengineargument. #22346 to pass these tests for now. Planning on revisiting this once rapidsmpf becomes the defaultOps-Bot-Merge-Barrier: true
Checklist